-
Notifications
You must be signed in to change notification settings - Fork 734
feat(cloudformation): Merge CloudFormation LSP integration with toolkit updates #8275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: feature/cloudformation
Are you sure you want to change the base?
feat(cloudformation): Merge CloudFormation LSP integration with toolkit updates #8275
Conversation
|
- Add CloudFormation Language Server Protocol support with multiple provider options - Include CloudFormation explorer with stack, resource, and change set management - Add stack deployment, validation, and change set workflows with S3 upload support - Include drift detection and diff visualization capabilities - Add CloudFormation environment and project management with cfn-init integration - Include telemetry and authentication handling - Add comprehensive test coverage for all CloudFormation features - Update package configurations and language syntax highlighting
f21322d to
09b66ad
Compare
laileni-aws
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the next time,
can we please raise short PR at regular intervals which will be easy to review!
Reviewing 114 files and 13K+ lines of code is tough and time consuming!
Thank you.
| return path.join(workspaceRoot, this.cfnProjectPath, this.environmentsDirectory, environmentName) | ||
| } | ||
|
|
||
| private async getConfigPath(): Promise<string> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we can move this to shared/utils
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will move to shared/utils
| const result = await resourcesManager.searchResource(node.label as string, identifier) | ||
|
|
||
| if (result.found) { | ||
| void window.showInformationMessage(`Resource found: ${identifier}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did we get sign off on these strings?
I guess we can be more elaborate on this information message or error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe our UX/Product has synced up with ToolKit team members to get confirmation on what we will show to customers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you cross check on this ?
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We received fit and finish approval today and this was one of the messages that was demonstrated. No issues raised.
| import { CloudFormationExplorer } from '../explorer/explorer' | ||
| import { commandKey } from '../utils' | ||
|
|
||
| export function selectEnvironmentCommand(explorer: CloudFormationExplorer): vscode.Disposable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need separate files for each command ? Instead we can have a single file like command.ts and include all commands inside and export them!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can consolidate to one commands file
|
|
||
| const DocumentPreviewNotification = new NotificationType<DocumentPreviewType>('aws/document/preview') | ||
|
|
||
| export class DocumentPreview { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be shared utils function!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless other parts of ToolKit needs this feature, is it worth moving it to the shared/utils?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is commonly used to show the preview but not a blocker for sure
| const message = | ||
| 'Help us improve the AWS CloudFormation Language Server by sharing anonymous telemetry data with AWS. You can change this preference at any time in aws.cloudformation Settings.' | ||
|
|
||
| const allow = 'Yes, Allow' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can improve readability
const THIRTY_DAYS_MS = 30 * 24 * 60 * 60 * 1000
const TELEMETRY_LEARN_MORE_URL = 'github.com/aws-cloudformation/cloudformation-languageserver/tree/main/src/telemetry'
interface TelemetryPromptOptions {
allow: string
later: string
never: string
learnMore: string
}
const PROMPT_OPTIONS: TelemetryPromptOptions = {
allow: 'Yes, Allow',
later: 'Not Now',
never: 'Never',
learnMore: 'Learn More'
}
const TELEMETRY_MESSAGE =
'Help us improve the AWS CloudFormation Language Server by sharing anonymous telemetry data with AWS. ' +
'You can change this preference at any time in aws.cloudformation Settings.'
```
…ents - Remove resource type from list using right click - Few startup fixes for CloudFormation extension - Add exec permissions and support node versions - Add node to AWS Explorer CFN panel - Add prompt for deploymentMode and plumb to deployment - Coordinate stack views and add overview to CFN console - Fix extract to parameter cursor command
…ents * Remove resource type from list using right click * Few startup fixes for CloudFormation extension * Add exec permissions and support node versions * Add node to AWS Explorer CFN panel * Add prompt for deploymentMode and plumb to deployment * Coordinate stack views and add overview to CFN console * Fix extract to parameter cursor command
| try { | ||
| const data = JSON.parse(context) | ||
| const pathParts = path.split('/').filter(Boolean) | ||
| let current: any = data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
Datatype is JSON
| )?.value | ||
| } | ||
|
|
||
| export async function getImportExistingResources(): Promise<boolean | undefined> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reuse above getIncludeNestedStacks function ?
| if (nodeA instanceof AWSCommandTreeNode) { | ||
| return -1 | ||
| } | ||
| if (nodeB instanceof AWSCommandTreeNode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( nodeA instanceof AWSCommandTreeNode || nodeB instanceof AWSCommandTreeNode) {
return 1
}
| label: 'test-profile', | ||
| state: 'valid', | ||
| } as any, | ||
| } as any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dataType: sinon.SinonStubbedInstance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will make updates to the tests
| @@ -0,0 +1,4 @@ | |||
| { | |||
| "type": "Feature", | |||
| "description": "CloudFormation: Add comprehensive Language Server Protocol integration with stack management, deployment workflows, drift detection, and cfn-init project support" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If required we can add multiple change logs.
laileni-aws
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to work on some nits and code refactoring but NOT a blocker for RIV:2025
| await this.updateCredentialsFromActiveConnection() | ||
| } | ||
|
|
||
| private async updateCredentialsFromActiveConnection(): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this always send the credentials or only when the feature is used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its hard to say if the feature is being used, because there are a lot of just UI elements visualizing their resources. Which requires sending credentials to load, they might not interact with the UI other than just viewing.
This will send the credentials anytime it changes, as long as the LSP is active
| } | ||
|
|
||
| public async getEnvironmentDir(environmentName: string): Promise<string> { | ||
| const workspaceRoot = workspace.workspaceFolders?.[0]?.uri.fsPath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this can be moved to a util function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will move to a util function
| parametersFiles?: string[] | ||
| } | ||
|
|
||
| export class CfnInitCliCaller { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this calling a dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the dependency is located inside the server code.
| await progress | ||
| void vscode.window.showInformationMessage(`CFN project '${this.state.projectName}' created!`) | ||
|
|
||
| const openProject = await vscode.window.showQuickPick(['Yes', 'No'], { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this already have pre-existing functions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No pre-existing functions for this case
| try { | ||
| // Show sign-in message when not authenticated | ||
| if (!globals.awsContext.getCredentialProfileName()) { | ||
| const signInNode = new PlaceholderNode(this as any, 'Sign in to get started') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would this allow all types of sign in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this just uses the existing login workflow
Problem
Solution
feature/xbranches will not be squash-merged at release time.